Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Expression and Statement proper subclasses of Node #2122

Closed
wants to merge 11 commits into from

Conversation

elazarg
Copy link
Contributor

@elazarg elazarg commented Sep 12, 2016

Resolves most of #1783 (except changing the parameters to NodeVisitor)

Serious change - supersedes PR #2121


# For now, only Var is a declaration
class Declaration(Node):
pass


class SymbolNode(Node):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly the first refactor that I would like to see is to make SymbolNode not derive from Node...

@gvanrossum
Copy link
Member

I think there's a lot of valuable stuff here. At the same time I don't like the need to add cast() in various places.

I wonder if this could be done in smaller steps, e.g. make the changes to checker.py and checkexpr.py without the other changes? They should still work with Expression and Statement just aliases, and you'd get a fair amount of the "more readable code". Then you can iterate on the rest, possibly implementing selftype first.

@elazarg
Copy link
Contributor Author

elazarg commented Sep 13, 2016

I don't like the cast either. But how does keeping the alias - effectively, cast() everywhere - better than a couple of explicit casts? There's no content to the Expression/Statement classes (which makes me think that a huge Union might be a better choice. is it?)

If the motivation for making only incremental change is affecting less lines of code at once, it's fine with me.

@gvanrossum
Copy link
Member

Looks like you can get rid of three casts by dropping the distinction between LvalueExpr and Expression. The union there doesn't sit well with me either, so maybe you can just get rid of of LvalueExpr for now (I know you have plans there, but please be patient).

One reason I dislike casts so much is that they incur real runtime overhead (it's a plain Python function and nothing optimizes it away).

Another reason is that we've had several occurrences where a cast was wrong (the 2nd argument was not an instance of the 1st argument) that were hard to debug.

I'd still like to see if we can do this more gradually -- I hate huge diffs, they are a pain to review and often get bogged down by (or cause) merge conflicts. So maybe one file at a time, with the complicated stuff last, if the tests can be made to pass in between?

@elazarg
Copy link
Contributor Author

elazarg commented Sep 14, 2016

I agree that casts must be obviously-correct, I will try to break this change to smaller pieces.

@elazarg
Copy link
Contributor Author

elazarg commented Sep 14, 2016

Turns out the Declaration thing does not hold water. I think there's some deep mix with Var, Argument, and SymbolNode-stuff. It should be sorted out, but not in this PR.

@gvanrossum
Copy link
Member

I have a suggestion. Maybe we can should start by doing the easy part only -- change most occurrences of Node to either Expression or Statement, but keeping those as aliases to Node for now. That will buy us the documentation benefit without having to worry about the subtler parts of the diff. We can then quickly approve the part of the diff that causes massive churn, and even if we end up not doing the other part ever it'll still be useful (if only as a hint). And the actual fix to make everything type-check correctly will be a much smaller diff which will make it easier to review (believe it or not) and less likely to go stale due to a merge conflict.

If you agree, you can choose to close this and start a new PR, or remove the tricky bits from this PR and resubmit it for review. Hope this helps!

@gvanrossum
Copy link
Member

Closing. See above.

@gvanrossum gvanrossum closed this Sep 29, 2016
@elazarg elazarg deleted the Expression_Subclass_Node branch October 27, 2016 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants